Enhancement: Public extension manager for ECH by sebastian-carpenter · Pull Request #10568 · wolfSSL/wolfssl

sebastian-carpenter · 2026-06-01T20:48:43Z

Description

Reworks ECH public-vs-private extension handling. The old path swapped a single SNI string in place on the live extension list - this was fragile and SNI-only. This PR generalizes it to a public-extension list on the ECH struct that's swapped by extension type, so the outer ClientHello can carry arbitrary public extensions.

ZD#21931

Public extension manager

Add TLSX* extensions to WOLFSSL_ECH holding the public extensions (e.g. the public-name SNI), populated in TLSX_ECH_Use from echConfig->publicName.
Replace TLSX_EchChangeSNI/TLSX_EchRestoreSNI with:
- TLSX_EchShouldHideInner - true when ECH is the outer type.
- TLSX_EchSwapExtensions - swaps matching extension types between ssl->extensions and ech->extensions; unmatched public exts are prepended; popCount reverses leading nodes to preserve OuterExtensions ordering.
- TLSX_EchReplaceExtensions - accept: free the public list; reject: swap public exts into ssl->extensions.
TLSX_GetSizeWithEch/TLSX_WriteWithEch install the public exts before size/write and swap back after.
ForceZero the hpke; free ech->extensions in TLSX_ECH_Free.

SNI changes

Remove the old SNI-state machinery: drop the EchStateSNI enum and the sniState/privateName fields, plus the sniState transitions in DoTls13HandShakeMsgType.
TLSX_SNI_Parse: drop the sniState-based private-name save/restore. On outer-CH parse, match against any echConfig->publicName and write a matched public SNI to ech->extensions, not ssl->extensions; simplify the matched/cacheOnly and response-flag logic.
Inner SNI is now optional - removed the client-side "inner SNI must be present" requirement enforced by TLSX_EchChangeSNI.
SendTls13ClientHello: read padding length via TLSX_SNI_GetRequest instead of ech->privateName.
Call TLSX_EchReplaceExtensions at the accept/reject points (EchCheckAcceptance, DoTls13ServerHello reject fallback, DoTls13ClientHello). This installs the correct SNI for use later in the handshake.

Testing

Add test_wolfSSL_Tls13_ECH_wire_sni: drive the handshake manually and assert the public name is present and the private name absent in raw CH1/CH2 bytes, across accept/reject.
Update test_wolfSSL_Tls13_ECH_no_private_name: no-inner-SNI now succeeds against a permissive server (ACCEPTED); rejected only with ABORT_ON_ABSENCE.
Add b64MandatoryFirst config case; test_ech_server_sni_callback rejects a wrong public name; add a double-public-SNI scenario to bad_configs_ex.
Remove obsolete test_wolfSSL_Tls13_ECH_long_SNI (its overflow target - the deleted swap helpers - no longer exist) and relocate the over-long-SNI BAD_LENGTH_E check into test_wolfSSL_UseSNI_params.

Added testing from #10542:

More efficient *_wire_sni test
Interop testing for when ECH is rejected

Follow-ups

Lays groundwork for managing multiple public extensions and multiple SNI's in the outer ClientHello.

Checklist

added tests
updated/added doxygen
updated appropriate READMEs
Updated manual and documentation

Copilot

Pull request overview

This PR refactors TLS 1.3 ECH public-vs-private extension handling by introducing a dedicated “public extensions” list on the WOLFSSL_ECH object and swapping extensions by type, replacing the previous SNI-only in-place string swap approach. It also updates SNI/ECH parsing and handshake flow to install the correct extensions on accept/reject paths, and adds/updates tests to validate wire visibility of public vs private SNI.

Changes:

Add WOLFSSL_ECH::extensions to hold “public” extensions and implement swapping/replacement helpers used during ClientHello size/write and accept/reject handling.
Rework SNI parsing and ECH handshake transitions to remove the old sniState/privateName machinery and support public-name matching for outer ClientHello.
Update API tests to cover new behaviors (wire-SNI visibility, no-inner-SNI behavior, new bad config cases) and relocate long-SNI length validation coverage.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
`wolfssl/internal.h`	Removes old SNI-state fields and adds `WOLFSSL_ECH::extensions` plus `TLSX_EchReplaceExtensions()` prototype.
`src/tls.c`	Implements public-extension swapping/replacement, updates SNI parsing, and updates ECH lifecycle management.
`src/tls13.c`	Adjusts ClientHello padding derivation and installs correct extension sets at ECH accept/reject points (incl. HRR).
`tests/api.c`	Adds/updates ECH/SNI tests (wire visibility, no-private-name behavior, new config cases) and relocates length checks.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

github-actions · 2026-06-08T18:34:07Z

retest this please

github-actions · 2026-06-08T22:43:37Z

MemBrowse Memory Report

gcc-arm-cortex-m4-pq

FLASH: .text +64 B (+0.0%, 277,208 B / 1,048,576 B, total: 26% used)

gcc-arm-cortex-m4-tls13

FLASH: .text +64 B (+0.0%, 234,464 B / 262,144 B, total: 89% used)

gcc-arm-cortex-m7-pq

FLASH: .text +64 B (+0.0%, 277,784 B / 1,048,576 B, total: 26% used)
No memory changes detected for:
gcc-arm-cortex-m0plus
gcc-arm-cortex-m3
gcc-arm-cortex-m4
gcc-arm-cortex-m4-baremetal
gcc-arm-cortex-m4-crypto-only
gcc-arm-cortex-m4-dtls13
gcc-arm-cortex-m4-min-ecc
gcc-arm-cortex-m4-openssl-compat
gcc-arm-cortex-m4-pkcs7
gcc-arm-cortex-m4-rsa-only
gcc-arm-cortex-m4-sp-math
gcc-arm-cortex-m4-tls12
gcc-arm-cortex-m7
gcc-arm-cortex-m7-tls13
linuxkm-pie
linuxkm-standard
stm32-sim-stm32h753

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

sebastian-carpenter · 2026-06-09T18:56:35Z

Jenkins retest this please.

dgarske

Skoll Code Review

Scan type: reviewOverall recommendation: COMMENT
Findings: 4 total — 4 posted, 0 skipped
4 finding(s) posted as inline comments (see file-level comments below)

Posted findings

[Medium] Multi-public-extension swap/reversal path is untested — src/tls.c:16682-16734
[Low] ECH padding length only considers ssl-extensions, not ctx-level inner SNI — src/tls13.c:4844-4855
[Info] Pointer declaration style inconsistent with repo convention — src/tls.c:2378
[Info] TLSX_EchShouldHideInner can be a single boolean expression — src/tls.c:16665-16671

Review generated by Skoll

dgarske · 2026-06-09T19:12:51Z

+ * popCount extensions are 'reversed' off the list.
+ *
+ * Returns a count of extensions prepended to sslExts. */
+static word16 TLSX_EchSwapExtensions(TLSX** sslExts, TLSX** echExts,


🟠 [Medium] Multi-public-extension swap/reversal path is untested

TLSX_EchSwapExtensions contains non-trivial pointer manipulation specifically designed for multiple public extensions: the popCount head-pop/reverse loop and the prepend-and-reverse ordering logic exist to preserve OuterExtensions ordering when more than one public extension is present. The PR body explicitly states this lays "groundwork for managing multiple public extensions and multiple SNI's". However, in the current code ech->extensions only ever holds a single SNI (populated in TLSX_ECH_Use and TLSX_SNI_Parse), so the multi-node popCount > 1 reversal path and the multi-prepend ordering are never exercised by any test. The new test_wolfSSL_Tls13_ECH_wire_sni only validates the single-SNI case. Since this is the most intricate part of the change, a regression here would be silent until the future multi-ext work lands.

Fix: Add a small unit test for the multi-extension swap/reverse ordering to lock in the reversal behavior the PR is building toward.

Added testing (more than a little...)

dgarske · 2026-06-09T19:12:51Z

            /* calculate padding (RFC 9849, section 6.1.3) */
-            if (args->ech->privateName != NULL) {
-                word16 nameLen = (word16)XSTRLEN(args->ech->privateName);
+            nameLen = TLSX_SNI_GetRequest(ssl->extensions,


🔵 [Low] ECH padding length only considers ssl-extensions, not ctx-level inner SNI

The replacement padding calculation reads the inner SNI via TLSX_SNI_GetRequest(ssl->extensions, ...). This only inspects ssl->extensions, whereas a client may configure its SNI at the CTX level via wolfSSL_CTX_UseSNI (which lands in ssl->ctx->extensions). In that configuration hostName will be NULL here and the code takes the paddingLen = maxNameLen + 9 branch as if no inner name were present. The effect is benign (more padding, never a buffer issue, and the actual outer SNI on the wire is still correct due to the semaphore override in TLSX_GetSize/TLSX_Write), but the padding no longer reflects the true inner name length, slightly weakening the length-hiding intent of RFC 9849 6.1.3 for ctx-level-SNI clients. The old code used ech->privateName. Worth confirming this is intentional / acceptable.

Fix: Confirm ctx-level inner SNI is out of scope, or add a ctx->extensions fallback for the padding length calculation.

Updated. Now checks the ctx if SNI is not found on the ssl

dgarske · 2026-06-09T19:12:51Z

    word16 offset = 0;
    int cacheOnly = 0;
    SNI *sni = NULL;
+    const char *hostName = NULL;


⚪ [Info] Pointer declaration style inconsistent with repo convention

The newly introduced declaration uses const char *hostName = NULL; (asterisk bound to the name), whereas the surrounding wolfSSL code and the original removed line used const char* hostName (asterisk bound to the type). Minor consistency nit only.

Fix: Match the const char* name pointer style used elsewhere in the file.

dgarske · 2026-06-09T19:12:51Z

-static int TLSX_EchChangeSNI(WOLFSSL* ssl, TLSX** pEchX,
-                             char* serverName, TLSX** pServerNameX,
-                             TLSX*** pExtensions)
+/* Returns 1 if the extensions should be hidden for this write */


⚪ [Info] TLSX_EchShouldHideInner can be a single boolean expression

The helper is a two-branch return of a simple predicate. It can be collapsed to a single expression for readability without changing behavior. Purely cosmetic.

Fix: Optionally simplify to a single return; take it or leave it.

- *_wire_sni test is now more efficient - openssl-ech workflow now does interop with ECH rejection extra improvements: - tested TLSX_EchSwapExtensions - added ctx level SNI to padding calculation

sebastian-carpenter self-assigned this Jun 1, 2026

Copilot AI review requested due to automatic review settings June 1, 2026 20:48

Copilot started reviewing on behalf of sebastian-carpenter June 1, 2026 20:48 View session

Copilot AI reviewed Jun 1, 2026

View reviewed changes

Comment thread src/tls.c

Comment thread src/tls.c

Comment thread tests/api.c Outdated

sebastian-carpenter mentioned this pull request Jun 8, 2026

CI Addition (ECH): Check only the public SNI is visible #10542

Closed

4 tasks

sebastian-carpenter force-pushed the tls-ech-ext-public branch from 2e125d1 to aa4831f Compare June 8, 2026 16:44

sebastian-carpenter marked this pull request as ready for review June 8, 2026 18:30

sebastian-carpenter force-pushed the tls-ech-ext-public branch 2 times, most recently from 8c2fa10 to 61e1638 Compare June 9, 2026 16:25

dgarske assigned wolfSSL-Bot Jun 9, 2026

dgarske requested a review from Copilot June 9, 2026 17:48

Copilot started reviewing on behalf of dgarske June 9, 2026 17:48 View session

Copilot AI reviewed Jun 9, 2026

View reviewed changes

Comment thread src/tls.c

Comment thread src/tls13.c Outdated

dgarske reviewed Jun 9, 2026

View reviewed changes

sebastian-carpenter added 5 commits June 9, 2026 22:43

ech public extension management

bc2fb23

add public sni handling to public ext

96cb4d1

minor fixes

bc7768c

move long_sni regression test

31ddc5b

testing improvements (from wolfSSL#10542):

4eef82d

- *_wire_sni test is now more efficient - openssl-ech workflow now does interop with ECH rejection extra improvements: - tested TLSX_EchSwapExtensions - added ctx level SNI to padding calculation

sebastian-carpenter force-pushed the tls-ech-ext-public branch from 61e1638 to 4eef82d Compare June 10, 2026 04:51

Conversation

sebastian-carpenter commented Jun 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Public extension manager

SNI changes

Testing

Follow-ups

Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Jun 8, 2026

Uh oh!

github-actions Bot commented Jun 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

MemBrowse Memory Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

sebastian-carpenter commented Jun 9, 2026

Uh oh!

dgarske left a comment

Choose a reason for hiding this comment

Skoll Code Review

Posted findings

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sebastian-carpenter commented Jun 1, 2026 •

edited

Loading

github-actions Bot commented Jun 8, 2026 •

edited

Loading